TRT-2647: rhcos10 default for 5.0#3498
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@neisw: This pull request references TRT-2647 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughVariant OS fallback for OpenShift 5.x/main changed from ChangesVariant OS default & related config/test updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-merge-bot
Bot
added
the
ready-for-human-review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp_test.go (1)
1139-1161: ⚡ Quick winConsider adding a no-release
-main-sibling case for branch-fallback coverage.You now validate
-master-with no release (rhcos10). Adding a matching-main-no-release case would close the parity gap forisMainBranch.✅ Suggested test addition
{ job: "periodic-ci-openshift-master-no-release-version-e2e-aws-ovn", expected: map[string]string{ VariantArch: "amd64", VariantInstaller: "ipi", VariantPlatform: "aws", VariantNetwork: "ovn", VariantNetworkStack: "ipv4", VariantOwner: "eng", VariantSuite: "unknown", VariantTopology: "ha", VariantUpgrade: VariantNoValue, VariantAggregation: VariantNoValue, VariantProcedure: "none", VariantJobTier: "candidate", VariantFeatureSet: VariantDefaultValue, VariantNetworkAccess: VariantDefaultValue, VariantScheduler: VariantDefaultValue, VariantSecurityMode: VariantDefaultValue, VariantCGroupMode: "v2", VariantLayeredProduct: VariantNoValue, VariantOS: "rhcos10", }, }, + { + job: "periodic-ci-openshift-main-no-release-version-e2e-aws-ovn", + expected: map[string]string{ + VariantArch: "amd64", + VariantInstaller: "ipi", + VariantPlatform: "aws", + VariantNetwork: "ovn", + VariantNetworkStack: "ipv4", + VariantOwner: "eng", + VariantSuite: "unknown", + VariantTopology: "ha", + VariantUpgrade: VariantNoValue, + VariantAggregation: VariantNoValue, + VariantProcedure: "none", + VariantJobTier: "candidate", + VariantFeatureSet: VariantDefaultValue, + VariantNetworkAccess: VariantDefaultValue, + VariantScheduler: VariantDefaultValue, + VariantSecurityMode: VariantDefaultValue, + VariantCGroupMode: "v2", + VariantLayeredProduct: VariantNoValue, + VariantOS: "rhcos10", + }, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/variantregistry/ocp_test.go` around lines 1139 - 1161, Add a sibling test case to the table in ocp_test.go that mirrors the existing "periodic-ci-openshift-master-no-release-version-e2e-aws-ovn" case but uses "periodic-ci-openshift-main-no-release-version-e2e-aws-ovn" as the job name to exercise the isMainBranch path; copy the expected map (VariantArch, VariantInstaller, VariantPlatform, VariantNetwork, VariantNetworkStack, VariantOwner, VariantSuite, VariantTopology, VariantUpgrade, VariantAggregation, VariantProcedure, VariantJobTier, VariantFeatureSet, VariantNetworkAccess, VariantScheduler, VariantSecurityMode, VariantCGroupMode, VariantLayeredProduct, VariantOS) from the master case so the assertion logic in the test (table-driven test around the job field) verifies parity for main-branch fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/variantregistry/ocp_test.go`:
- Around line 1139-1161: Add a sibling test case to the table in ocp_test.go
that mirrors the existing
"periodic-ci-openshift-master-no-release-version-e2e-aws-ovn" case but uses
"periodic-ci-openshift-main-no-release-version-e2e-aws-ovn" as the job name to
exercise the isMainBranch path; copy the expected map (VariantArch,
VariantInstaller, VariantPlatform, VariantNetwork, VariantNetworkStack,
VariantOwner, VariantSuite, VariantTopology, VariantUpgrade, VariantAggregation,
VariantProcedure, VariantJobTier, VariantFeatureSet, VariantNetworkAccess,
VariantScheduler, VariantSecurityMode, VariantCGroupMode, VariantLayeredProduct,
VariantOS) from the master case so the assertion logic in the test (table-driven
test around the job field) verifies parity for main-branch fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 683d6765-f56e-45e2-9847-79272263d51c
📒 Files selected for processing (2)
pkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.yaml
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-rebase
neisw
force-pushed
the
trt-2647-5.0-rhcos10-default
branch
from
72e7de3 to
84a1567
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-ci
Bot
removed
the
needs-rebase
|
Scheduling required tests: |
dgoodwin
left a comment
dgoodwin
left a comment
There was a problem hiding this comment.
Everything else looks good.
Holding until we hear it's flipped sounds good.
| enabled: false | ||
| regression_tracking: | ||
| enabled: true | ||
| enabled: false |
There was a problem hiding this comment.
This is interesting, looks like it flipped several settings to the "calm things down for a GA branch", is that intentional for the 9 vs 10 5.0 view? I would probably keep regression tracking on as well.
There was a problem hiding this comment.
@dgoodwin your jira card TRT-2647 indicates
Add an rhcos9 vs 10 cross compare view instead to minimally monitor that 9 doesn’t get worse than 10. Probably could detune it the way we do for ga -main views now. We don’t want to be overly sensitive.
I can enable regression_tracking here if you like. Just let me know what you want the final config to look like.
There was a problem hiding this comment.
yes tracking would be useful here.
openshift-ci
Bot
added
the
needs-rebase
# Conflicts: # pkg/variantregistry/snapshot.yaml
# Conflicts: # pkg/variantregistry/snapshot.yaml
neisw
force-pushed
the
trt-2647-5.0-rhcos10-default
branch
from
84a1567 to
e15f740
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
Scheduling required tests: |
| @@ -1313,8 +1313,7 @@ func setOS(_ logrus.FieldLogger, variants map[string]string, jobName string) { | |||
| case variants[VariantReleaseMajor] == "4": | |||
| variants[VariantOS] = "rhcos9" | |||
| case variants[VariantReleaseMajor] == "5" || isMainBranch: | |||
There was a problem hiding this comment.
we'll need the special treatment for update jobs as discussed on slack, right?
|
Scheduling required tests: |
|
@neisw: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Staging for when the default changes over to rhcos10 for 5.0
Open question, should we remove
OS:from the 5.0 main view so they are all picked up by default or explicitly specify rhcos9 and rhcos10?Summary by CodeRabbit
New Features
Chores
Tests